Further clean up usages of LowLevelList<T> #109114

huoyaoyuan · 2024-10-22T13:01:29Z

Use List<T> outside type loader, and replace the usage of LowLevelList<T> to bool[] for representing field GC layout.

Separated with LowLevelDictionary<T>, which may introduce more concern about code size and speed.

dotnet-policy-service · 2024-10-22T13:02:14Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

huoyaoyuan · 2024-10-22T13:02:31Z

src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeBuilder.cs

            /// <param name="offset">The offset at which we need to write the bitfield.</param>
-            public void WriteToBitfield(LowLevelList<bool> bitfield, int offset)
+            /// <returns>The result bitfield, may or may not be the input array.</returns>
+            public bool[] WriteToBitfield(bool[] bitfield, int offset)


I'm not sure how relevant is this. I can only find one caller of this method which builds ArrayType.

I am not sure what you mean by "this"

I mean the WriteToBitfield method and its descendants. I can only find one caller and its input argument is always empty. The GCLayout structure represents a mergeable bit vector, but I can't find the callers fully leveraging it.

Feel free to simplify it

Taking more insight on GCLayout, it's converting two representations back and forth. I'm not fully understood about GC info encoding and not sure whether it should be converted further.

huoyaoyuan · 2024-10-22T14:13:31Z

src/coreclr/nativeaot/Common/src/System/Collections/Generic/LowLevelList.cs

-#if TYPE_LOADER_IMPLEMENTATION
    [System.Runtime.CompilerServices.ForceDictionaryLookups]
-#endif
    internal class LowLevelList<T>


Should the type be moved under TypeLoader since it's only required for it?

Additional question: is ForceDictionaryLookups only applicable for reference types because of generic sharing, or also applicable for value types?

Should the type be moved under TypeLoader since it's only required for it?

It is very likely that there is code in CoreLib that is used by the TypeLoader indirectly that would need to be annotated with this attribute if we ever resurrected this (#85184 (comment)).

I think it would be ok to delete ForceDictionaryLookups and its few remaining uses. It is likely heavily bitrotten.

Yeah, in this PR I was using Array.Resize which is generic. It seems that the real foreseeing solution is to limit TypeLoader usage of CoreLib, according to the linked discussion. It also means that TypeLoader needs its specialized collection. If we resurrected the generic lookup work, is it expected to use the attribute, or just do some specialized handling for TypeLoader?

The original question is that, would there be any place else (not involved with TypeLoader) unable to use the CoreLib collection? I assume no.

Per #85184 (comment), if we ever resurrected the lazy generic lookup work, we would need likely ended up with TypeLoader in its own partition that does not perform the lazy generic lookups. It would be very large refactoring.

huoyaoyuan · 2024-11-17T18:48:29Z

...on/src/Internal/Reflection/Execution/ExecutionEnvironmentImplementation.ManifestResources.cs

        /// The dictionary's value is a list of (resourcename,index) tuples.
        /// </summary>
-        private static volatile LowLevelDictionary<string, LowLevelList<ResourceInfo>> s_extractedResourceDictionary;
+        private static volatile LowLevelDictionary<string, List<ResourceInfo>> s_extractedResourceDictionary;


In avialonia-linux, all interfaces on the List<ResourceInfo> are preserved. This is causing the majority of size difference.

Appearantly the usual List and Dictionary do have higher size impact than LowLevelList and LowLevelDictionary, but they are very likely to be used by other part of application, especially Dictionary<__Canon, __Canon>.

agocke · 2025-03-03T21:06:35Z

cc @MichalStrehovsky

huoyaoyuan · 2025-05-26T12:46:44Z

Triaging the status of this PR: we should first make the footprint for List<ValueType> smaller so that it can be adopted more easily. We may also switch element types to class for cold path, as List<ReferenceType> is expected to be extremely common.

jkotas · 2025-05-26T16:01:09Z

...nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerServices/FunctionPointerOps.cs

        private static uint s_genericFunctionPointerNextIndex;
        private const uint c_genericDictionaryChunkSize = 1024;
-        private static LowLevelList<IntPtr> s_genericFunctionPointerCollection = new LowLevelList<IntPtr>();
+        private static readonly List<IntPtr> s_genericFunctionPointerCollection = new List<IntPtr>();


This s_genericFunctionPointerCollection pool looks like a premature optimization. I think it would be fine to allocate GenericMethodDescriptorInfos one at a time.

Does this mean there will be memory leak per function pointer, instead of per method instantiation? This would become unbounded instead of bounded.

s_genericFunctionPointerCollection does not enable any GenericMethodDescriptor reuse. There is s_genericFunctionPointerDictionary for that. I am not suggesting deleting s_genericFunctionPointerDictionary.

s_genericFunctionPointerCollection reduces frequency of NativeMemory.Alloc calls. We allocate spaces for c_genericDictionaryChunkSize GenericMethodDescriptors at a time using NativeMemory.Alloc and then hand them out one at a time.

jkotas · 2025-05-26T16:04:14Z

Triaging the status of this PR: we should first make the footprint for List smaller so that it can be adopted more easily.

That's very hard.

I would split this PR into multiple smaller ones that are easier to discuss, review and merge:

Delete ForceDictionaryLookups attribute from everywhere
bool[] refactoring in type loader
...

jkotas · 2025-05-26T23:54:59Z

...on/src/Internal/Reflection/Execution/ExecutionEnvironmentImplementation.ManifestResources.cs

            {
                if (s_extractedResourceDictionary == null)
                {
                    // Lazily create the extracted resource dictionary. If two threads race here, we may construct two dictionaries


I think the question to ask here is whether we need to create this global cache in the first place.

agocke · 2025-05-31T15:54:23Z

I'm going to move this to draft for now. Can be resubmitted with more granular changes.

dotnet-policy-service · 2025-06-30T19:20:55Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

huoyaoyuan added 6 commits October 22, 2024 18:48

Use List<T> in CoreLib

d465850

Use List<T> in ReflectionExecution

a672cdf

Replace with bool[] in GC layout

0bfd0cb

Update documentation and remove unused members

46546e6

Remove unused _version

8d45b8d

Replace usage of ref array

b1aa7db

huoyaoyuan requested a review from MichalStrehovsky as a code owner October 22, 2024 13:01

ghost added the area-NativeAOT-coreclr label Oct 22, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 22, 2024

huoyaoyuan commented Oct 22, 2024

View reviewed changes

huoyaoyuan added 3 commits October 25, 2024 00:34

Merge branch 'main' into lowlevel-list-2

4bc86fe

Remove offset and input for bitfield

2aa0f67

Merge branch 'main' into lowlevel-list-2

e561f32

github-actions bot mentioned this pull request Nov 15, 2024

109114 MichalStrehovsky/rt-sz#78

Closed

huoyaoyuan commented Nov 17, 2024

View reviewed changes

huoyaoyuan added 2 commits January 4, 2025 20:15

Merge branch 'main' into lowlevel-list-2

cbfe20c

Use Dictionary instead

9bcd4dc

github-actions bot mentioned this pull request Jan 4, 2025

109114 MichalStrehovsky/rt-sz#94

Closed

This was referenced Jan 4, 2025

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

Merge branch 'main' into lowlevel-list-2

5686096

This was referenced Mar 24, 2025

System.Net.Quic.Tests failed with "System.Net.Quic.QuicException : The connection timed out from inactivity." #105177

Closed

System.Net.Quic tests timeout #107761

Closed

jkotas reviewed May 26, 2025

View reviewed changes

agocke marked this pull request as draft May 31, 2025 15:54

This was referenced Jun 4, 2025

Cleanup LowLevelList usage in GCLayout #116316

Merged

Delete manifest resource cache for NativeAOT #116471

Merged

dotnet-policy-service bot closed this Jun 30, 2025

jkotas mentioned this pull request Jun 30, 2025

Delete ForceDictionaryLookups #117180

Merged

huoyaoyuan deleted the lowlevel-list-2 branch July 2, 2025 06:09

github-actions bot locked and limited conversation to collaborators Aug 5, 2025

Further clean up usages of LowLevelList<T> #109114

Further clean up usages of LowLevelList<T> #109114

Uh oh!

Conversation

huoyaoyuan commented Oct 22, 2024

Uh oh!

dotnet-policy-service bot commented Oct 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke commented Mar 3, 2025

Uh oh!

huoyaoyuan commented May 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented May 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke commented May 31, 2025

Uh oh!

dotnet-policy-service bot commented Jun 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants